Skip to content

opcua: rescan while disconnected, heal PLC_COMMS_LOST on connect, and name a plugin freeze-frame's source - #660

Open
bburda wants to merge 32 commits into
mainfrom
fix/opcua-rescan-comms-lost-freeze-source
Open

bburda wants to merge 32 commits into
mainfrom
fix/opcua-rescan-comms-lost-freeze-source

Conversation

@bburda

@bburda bburda commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

In config-less discovery mode (OPCUA_DISCOVERY_ENABLED=1, subnets set, no
OPCUA_ENDPOINT_URL, no node map), a gateway that boots before its controller never
finds it. The start-up scan runs while the controller is still coming up, finds nothing,
falls back to opc.tcp://localhost:4840 and retries that endpoint until someone
restarts it. Three more things read wrong beside it: the gateway lists its three helper nodes
as apps, a plugin entity's freeze-frame names no origin, and a
PLC_COMMS_LOST raised before a restart stays CONFIRMED, because only the process that
raised it would clear it.

What changes

Discovery and reconnect

  1. With no session up and no endpoint_url configured, the reconnect arm rescans on
    discovery.interval_s, adopts a server that appeared after start-up and resets the
    backoff. The cadence is measured from the end of a sweep, the backoff is capped at it
    so the documented interval is the real one, and an unchanged outcome reports at DEBUG.
    The plugin is bound to the ApplicationUri of the server a session reached: the start-up
    sweep and every rescan look for that server at any address and refuse every other, and a
    different server answering at the bound address is dropped at connect with the outage
    left standing. The binding is written to discovery.binding_file (default
    /var/lib/ros2_medkit/opcua/binding, env OPCUA_DISCOVERY_BINDING_FILE, empty
    disables) when it is established and read at start, so a restart keeps the identity;
    removing the file and restarting is the recommissioning gesture. The first adoption of a
    process with no file, a configured endpoint_url and a server that publishes no
    ApplicationUri are outside the binding. One file belongs to one plugin instance.
    The reconnect arm owns every reconnect: after each client iterate the session state is
    read, a session the library re-opened underneath is dropped and handed to the arm, which
    connects through its own path, runs the binding check and re-creates the alarm
    subscription. Every path that marks the client disconnected drops the session's
    subscription bookkeeping, so the re-created monitored item delivers again.
  2. Both sweeps take a cancel predicate that reads the node's own context. A rescan that
    throws is caught on the poll thread, logged at WARN, and still stamps the cadence.
  3. With no node map, the component identity is re-derived from the device once a session
    is up, before the session subscribes to events, so the provisional opcua-<host> name
    goes and the conditions the server replays land on the new entity. A name the device
    gave itself is never replaced by the fallback; an empty read is retried a bounded
    number of times per session (one read at connect, up to three more in the connect
    hook, then at most five on later polls). Conditions the poller already pinned under
    the old alarms entity are moved to the new one; the event path reads a copy of the
    alarm routing owned by the poller, refreshed from the poll thread.

Comms-lost clears and the pending buffer

  1. On every successful connect the plugin asks the fault manager who reported a standing
    PLC_COMMS_LOST and clears it only when every reporting source is an id this bridge
    reports under: the current component id and the opcua-<host> stand-ins it assigned
    in this process. A row another bridge holds is left standing. The decision runs on the
    poll thread, only while the link is up, with one fault-store read outstanding at a
    time; a read the store does not answer within fault_service_timeout_ms (new key,
    default 5000) is dropped and taken again, and an answer that arrives after the link
    dropped is held until the next connect. A link-state clear the bounded buffer could
    not keep is owed and decided again once the sink is ready.
  2. Link-state clears and the scoped operator clear set skip_correlation_auto_clear. A
    clear the device itself reports still cascades.
  3. The buffer used while fault_manager is unreachable keeps the PLC's order: a clear is
    coalesced onto a pending clear for the same code only when nothing was reported for
    that code in between, so report, clear, report, clear flushes in that order. A device
    alarm's inactive edge ages out like a report.

Gateway entity listing

  1. One predicate names the gateway's three helper nodes (<node>_sub,
    <node>_fault_clients, <node>_lifecycle_state_reader) for the peer count and the app
    filter, matched per exact suffix so a peer named <node>_monitor or <node>2 stays
    visible. The gateway's own node stays an App: its parameters are that App's
    configurations. _sub follows the gateway's namespace; the other two are created in the
    root namespace, so a helper-named node there counts as plumbing whichever gateway created
    it. While discovery.runtime.filter_internal_nodes is true the same predicate keeps the
    helpers out of /health's linking.orphan_count and out of the unmanifested_nodes
    warning; with the flag false they are served as Apps and counted there like any other
    undeclared node. A manifest App bound to a helper is dropped with one warning per
    distinct set. The peer count excludes _lifecycle_state_reader. Remote entities are
    untouched.

Freeze-frame source

  1. An entity frame carries x-medkit.source, plugin_data_provider or
    plugin_x_plc_data_route. topic and message_type stay empty, and the field is
    omitted when the capture named no path.

Docs and changelog

  1. Plugin README, docs/api/rest.rst, docs/tutorials/snapshots.rst,
    docs/config/discovery-options.rst.

Behaviour changes to know about

  1. interval_s unset means the 30 s default, an explicit 0 means discovery on with no
    rescan. The old README line was interval_s: 0, so a config copied from it gains no
    rescanning on upgrade. Drop the key for the default. A negative value is refused.
    This one is breaking.
  2. A per-entity DELETE /{entity}/faults/{code} served by this plugin now sets
    skip_correlation_auto_clear, as the ClearFault contract already promised.
  3. Three /api/v1/apps entries disappear with the default filter_internal_nodes: true:
    the gateway's helper nodes. The gateway's own node stays.
  4. A SIGTERM ends a start-up or rescan sweep within one probe.
  5. A PLC_COMMS_LOST that two bridges raised into one fault manager is not cleared by
    either link coming back; an operator clears it. The fault manager has no per-source
    de-assert today.
  6. New plugin config key fault_service_timeout_ms (default 5000, out of range refused
    with a warning). comms_lost_debounce_ms logs a warning when it clamps.
  7. New plugin config key discovery.binding_file. The identity survives a restart; a
    replaced PLC at the bound address is refused until the file is removed. In the shipped
    image the directory exists but persists across a re-created container only when mounted.
  8. A PLC swapped or rebooted under a config-less session with native alarms is now
    noticed: the outage is reported and the alarm subscription is re-created on the bound
    server when it returns. Before, the client library re-opened the session silently and
    device alarms stayed dead until a restart.

Testing

  1. Jazzy, clean tree: the opcua unit suite (405 tests) and the gateway unit suite pass with 0 failures, and the full integration package passes 1242 tests with 0 failures; the four
    contract tests that failed on every distro (test_configuration_api,
    test_locking_disabled_contract, test_relay_peer_credential,
    test_unmanifested_policy) pass, with three new feature tests beside them
    (test_own_node_apps, test_own_node_apps_namespaced, test_own_node_undeclared)
    that fail when the helper filter or the own-node rule is broken on purpose.
    test_operation_handlers pins the stop response on the contract (goal CANCELING or
    CANCELED), 10 of 10. The alarm-server test fixture runs every
    UA_Server_* call on the server's own thread, as UA_MULTITHREADING 0 requires, and
    the identity tests tear their executor down on every exit path.
  2. Plugin e2e against the fixture server with stub fault services: a fault raised under
    the stand-in heals after the device names itself; a row two bridges hold is left
    standing; no decision is taken while the link is down; a probe the store never answers
    is dropped and taken again; an answer to a timed-out probe is not read as the next
    one's; a link-state clear the buffer refused is sent last once the sink is ready; the
    connect hook runs before the event routing is copied; a rename moves the conditions
    already pinned. Unit tests pin the gate predicate, the decision predicates, the buffer
    order over every report/clear interleaving of length four, the identity rule, the read
    budget and the burst cadence, the guarded rescan, the node-context cancel, and the
    linker skip in both flag positions. Each new test was made to fail first. The binding
    file has unit tests for its format (one line, BOM, control characters, CRLF, a second
    line, a symlinked path, an unwritable parent) and e2e tests against the fixture: a
    restarted plugin keeps the binding and refuses a foreign server at the bound address, a
    configured endpoint_url ignores the file, the start-up sweep selects the bound server
    and not the lowest address, a swap and an outage under a config-less session are caught,
    and a device alarm is delivered after the bound server reboots.
    test_openapi_contract sweeps every entity, requires a built item for every listed
    resource, checks the item each one names, and waits for calibration's operation through
    the shared class wait.
  3. Docker: run_integration_tests.sh Total: 18 / Passed: 18, run_alarm_tests.sh
    passed, run_discovery_race_test.sh passed. That race scenario fails on the merge
    base at step 6 of 9, the adoption assertion (endpoint still 'opc.tcp://localhost:4840' (connected='False') after 50s), and passes on the branch
    through step 9 of 9, the config-less rename, which now also asserts the stand-in
    component left /components.
  4. Scoped clear live: PLC_TANK_HIGH still CONFIRMED after PLC_COMMS_LOST was cleared
    through the per-entity route. The control with the flag off cascades.
  5. SIGTERM 14 s into a start-up sweep: container gone in 506 ms, against 98 s
    uncancelled.
  6. TSan is verified by CI only: a sanitizer build of this package alone links against an
    uninstrumented rclcpp and is not usable on the development machine.

Not in this PR

  1. A docker scenario that restarts the gateway with a standing PLC_COMMS_LOST. That
    case has plugin-level e2e coverage with stub fault services.
  2. An empty source_id on PLC_COMMS_LOST in the /api/v1/faults list view while the
    fault manager logged a source. Unverified, filed on its own if it holds.
  3. gateway_node exiting 139 on shutdown with the plugin loaded. It reproduces on main,
    see [BUG] gateway_node segfaults on shutdown when the OPC UA plugin is loaded #659.
  4. A per-source de-assert in the fault manager, which is what would let two bridges heal
    one shared PLC_COMMS_LOST row.

Issue

Type

  • Bug fix
  • New feature or tests
  • Breaking change
  • Documentation only

Checklist

  • Breaking changes are clearly described (and announced in docs / changelog if needed)
  • Tests were added or updated if needed
  • Docs were updated if behavior or public API changed

Two failures share the same shape: the plugin decides something once, at
startup, and can never revise it while it runs.

Re-scan while no session is up. Config-less discovery (#544, #509) ran a
single scan a couple of seconds after start. A gateway that boots
alongside its PLC scans while the PLC is still coming up, finds nothing,
falls back to opc.tcp://localhost:4840 and retries that endpoint for as
long as it runs. Only a restart found the PLC. The poller's reconnect arm
now asks the plugin for a fresh scan, rate limited by discovery.interval_s
(default 30 s), and adopts a newly found server for its next connect
attempt, resetting the backoff so the new endpoint is tried at once rather
than after the dead one's accumulated wait. The rules that made discovery
safe are unchanged: an explicitly configured endpoint_url still wins and
is never rescanned, the scan stays a bounded read-only TCP sweep plus
GetEndpoints, and nothing is scanned while a session is up. interval_s
stops being an accepted-but-ignored knob.

Clear PLC_COMMS_LOST on every successful connect. The fault raised for a
sustained outage (#496) was cleared only when the running process still
remembered raising it. The fault manager keys faults by fault code and
persists them, so a fault raised before a gateway restart is standing in
the store with nothing in memory to remember it, and the arm that would
clear it is never entered when the first connect succeeds. The fault then
stayed CONFIRMED for good. Both connect paths, the initial one and every
reconnect, now send the clear regardless of what this process raised. The
clear is fire and forget, so a clear for a fault that is not there costs
nothing. The debounce that governs raising is untouched.

Tests: the discovery pass and the endpoint adoption rule are exercised
with injected probes, including the positive control that a configured
endpoint is refused the very server an unconfigured one accepts. The
comms-lost heal runs against the live test server, because only a connect
that actually succeeds reaches that arm.
A freeze-frame captured for a plugin-backed entity (#564) reaches a client
with an empty topic and an empty message_type. That is correct, the values
are the plugin's live entity data and not a ROS message, but it leaves the
snapshot with no field at all naming its origin. Two of these frames from
different bridges are indistinguishable, and a frame is indistinguishable
from a topic capture whose metadata went missing.

Frame now carries the capture path that read it, and it is served as
x-medkit.source: plugin_data_provider for a read through the owning
plugin's DataProvider, plugin_x_plc_data_route for the in-process dispatch
of the plugin's own x-plc-data route (bridges that export no
DataProvider). topic and message_type are left empty rather than
overloaded, since neither names a ROS topic here.

The field is omitted, not emptied, when the capture named no path, so a
fault-manager freeze-frame taken from a real topic is unaffected and
carries its topic and message_type as before.
The gateway runs four nodes inside its own process, all named after
itself: the gateway node, "<node>_sub" for the subscription executor,
"<node>_fault_clients" for the fault-service transport, and
"<node>_lifecycle_state_reader" for the lifecycle reader. None of them
starts with an underscore, so the ROS 2 hidden-node convention does not
cover them, and runtime introspection returned all four as ordinary Apps.
The gateway advertised its own plumbing as diagnosable entities, and an
operator browsing /api/v1/apps saw four entries that answer nothing
useful.

There were two half-answers to the same question. count_peer_nodes knew
the gateway's own FQN plus "_sub" and "_fault_clients" but not the
lifecycle reader, and the app filter knew only the underscore rule, so it
dropped none of the four. Both now go through one predicate,
is_own_gateway_node, so a fifth helper is declared in one place instead
of two.

The match is exact per suffix, never a prefix test: a genuine peer named
"<node>_monitor" or "<node>2" must stay visible, and hiding a real node is
the worse error. A fault_manager sharing the process is not ours either
and stays listed. Remote entities are left alone: a peer's helper nodes
carry the same fully qualified names and are the peer's own filter's
business.
Every existing opcua docker scenario pins OPCUA_ENDPOINT_URL, which
short-circuits discovery, so none of them can reach the failure this
covers: the gateway and the PLC power on together, the start-up scan runs
while the PLC is still booting, and the plugin is left retrying its
fallback endpoint.

The scenario starts the gateway first with discovery on and no endpoint
configured, asserts it settled on the fallback endpoint with no session,
then brings an OPC-UA server up on the same subnet and asserts the
endpoint is adopted within two re-scan intervals. It also asserts the
container never restarted, since a restart would satisfy the endpoint
check while proving nothing: a restart is exactly what used to be needed.

The network is created with an explicit /24 so the read-only sweep stays
254 hosts and finishes in seconds.
…cted

A gateway that starts before its PLC runs the discovery rescan for the
life of the outage, and several things it does in that state were wrong.

Component identity. With no node map the SOVD component is named from
the device. When the start-up connect fails there is no device to ask,
so the name comes from the fallback endpoint and an empty DeviceInfo,
and it was then pinned forever: after discovery adopted the real PLC the
component still served opcua-<fallback host> while introspect() reported
the adopted endpoint. The identity is now re-derived on the first poll
of a new session (config-less mode only, an explicit node map still owns
the name), the derived alarms entity follows the rename, and the change
is logged at INFO. The docker race scenario gained a config-less pass
that asserts the rename against a real server.

Rescan cadence. The cadence was stamped when a sweep STARTED, so a sweep
of a legal /16 (minutes at the defaults) made the next one due the
moment it returned: the poll thread swept back to back and the reconnect
attempt dropped to one per sweep. It is now stamped when the sweep ends.
NetworkDiscovery::run() also takes a cancel predicate, bound to the
shutdown flag and checked before each probe and between the sweep and
identify phases, so shutdown() no longer has to wait out a sweep.

Reconnect backoff. The rescan is consulted once per reconnect attempt
and attempts are spaced by the exponential backoff, so the real cadence
was max(interval_s, backoff) while the README, the header and the
start-up log all said "every interval_s". The backoff ceiling is now
capped at the rescan cadence while discovery is rescanning.

interval_s. An unset interval and an explicit 0 both mapped to the 30 s
default, so there was no way to keep discovery on and stop rescanning.
They are now distinct: unset takes the default, an explicit 0 leaves the
start-up scan one-shot, and a negative value is refused with a warning
that no longer claims the kept default is one-shot. The start-up line no
longer promises a loop that rescans "every 0s" either.

Discovery report. A pass re-emitted its whole report every rescan, so a
site with a secured-only server logged the same WARN every 30 s for the
life of the process. A pass whose outcome matches the previous one now
reports at DEBUG. The first pass, and every changed outcome, still
reports at INFO/WARN.

Connect-time clear. It is a link-state clear, not an operator resolving
a root cause, so it now sets skip_correlation_auto_clear and cannot
cascade-clear the symptom faults a rule attributes to PLC_COMMS_LOST.
The poller's own clear on a successful reconnect is the same event and
does the same. Those clears were also buffered unconditionally while the
fault manager was unmatched, so a flapping link pushed real alarm
reports out of the bounded buffer: the buffer now keeps at most one
pending clear per fault code, evicts a clear before a report, and
refuses a clear rather than dropping a report.
The two Frame::source constants had no assertion from a capture: only a
hand-built frame in the merge helper's test named one, so swapping the
DataProvider and route values left the whole suite green. The route and
DataProvider loss-of-comms tests now each assert the constant their own
path must produce, plus the literal wire value - symbol against symbol
stays equal when the two constants are swapped, and that string is what
every x-medkit.source consumer reads. The merge test that omits the key
keeps its place as a helper contract for a frame a caller built without
naming a path - the capture paths always name one - and says so instead
of standing in as a control for them.

The peer-node count test listed only two of the three helper nodes the
gateway creates in its own process, so a lone gateway with a lifecycle
reader would have counted a peer and skipped the empty-graph warning.

Document the entity-frame source field in the REST fault snapshot
reference: an entity frame carries no topic or message type, so source
is the only provenance a consumer gets.
…y what is re-derivable

The per-entity SOVD route DELETE /{entity}/faults/{code} lands on
FaultProvider::clear_fault for a plugin-owned entity, which is the branch
the gateway takes instead of its own. The gateway sets
skip_correlation_auto_clear there so an operator scoped to one entity
cannot cascade-clear correlated symptoms reported by apps in other
entities, and the ClearFault contract documents that guarantee, but this
plugin sent the flag off and reopened the hole wherever a PLC is
involved. It now sets the flag, and the reason each call site sets or
clears it travels with the call: a ClearOrigin says whether the device
reported the condition inactive (a real resolution, cascade kept), the
link came back, or an operator cleared through the scoped route.

The pending-dispatch buffer used that same distinction too bluntly. It
gave up any clear before any report, but only the link-state clear is
re-derivable - the next reconnect sends it again. A device alarm's
inactive edge is as one-shot as its raise, so evicting it left the flush
replaying the raise with nothing behind it and the fault standing while
the device said inactive. Ranking is now by re-derivability: the
link-state clear is what a full buffer gives up first, everything else
ages out oldest-first as it did before.

The start-up discovery sweep claimed to be cancellable through the
shutdown flag, but nothing can set that flag while it runs: it happens
inside set_context(), during node construction, before the executor the
gateway shuts down from ever spins. A SIGTERM during a wide sweep
therefore waited the sweep out. Both sweeps now ask one predicate that
also reads rclcpp::ok(), which rclcpp's own signal handler turns false,
so the start-up sweep ends on the signal and the rescan keeps ending on
shutdown() as well.

Also: a rescan sweep that throws now stamps the cadence on its way out,
or the next poll iteration would immediately start another one; the
start-up log line no longer states the wrong reason for reporting in
full; and the package changelog records this branch.

Measuring that start-up sweep also showed the gateway logging nothing at
all while it ran: the discovery report is buffered so a repeated rescan
can be reported at DEBUG, and the "scanning [subnets]" announcement had
been swept up with it. A minutes-long sweep with no output reads as a
hung process, so the announcement is sent before the sweep again, with
the same first-pass INFO / rescan DEBUG levelling on its own.
…e wire

Four call sites decide whether a ClearFault may cascade, and only the
scoped SOVD DELETE was pinned: swapping the origin at any of the other
three left the suite green. The two that need a live session are now
driven against the test_alarm_server fixture with a real fault manager on
the other end, so the flag is read off the wire. A successful connect
must clear PLC_COMMS_LOST without cascading, and the same test drives the
fixture's own CLI to fire and clear a condition, because a clear the
device itself reported is a resolution at the source and must keep the
cascade. Having both cases in one test is what makes each flag a decision
rather than a constant. The fixture harness gained a stdin pipe to send
those commands, the way the docker scenario already drives it through a
FIFO.

The poller's own clear travels the same callback as every device alarm,
so the rule that tells them apart moved into clear_origin_for_signal and
is tested on both branches, exact code match included.

The sweep's cancel predicate had the same shape of hole: it is private
and no test reached it, so reverting it to the shutdown flag alone left
everything green while a SIGTERM during the start-up sweep would again
have to wait the sweep out. The rule is now the static
discovery_cancelled_for, tested on both inputs, with the member reduced
to reading the two values off the process. A second test shows the input
is real by shutting a private rclcpp context down and reading
rclcpp::ok() back. The comment on the cancellation test no longer claims
it exercises the plugin's own predicate, which it never did.

Also: the remaining prose semicolons on this branch (two comments and
five operator-visible log strings) are periods and commas now, and the
Refused outcome's doc comment says what it means.
The app filter removes only the helper nodes the gateway runs inside its
own process - "<gateway>_sub", "<gateway>_fault_clients" and
"<gateway>_lifecycle_state_reader". The gateway's own node stays an App:
its ROS parameters are what SOVD serves as that App's configurations, and
two gateways watching one graph have to agree on what is on it.

A feature test launches a gateway with its real process node names - the
suite's usual launch remaps all four to one string - and pins both halves:
the gateway is listed, addressable and configurable, and each helper is on
the graph but absent from the app list.
An accepted stop leaves the goal CANCELING or CANCELED, and which of the
two a caller observes depends on how fast the server winds down. The
body renders the status the handler read, which is at or before the one
the test reads afterwards, so a goal still CANCELING pins the body
exactly while a goal already CANCELED admits either rendering. What stays
ruled out is a goal that is still running or completed anyway.
The e2e tests that drive the plugin against a live fault-manager stub spin
a MultiThreadedExecutor on a thread of their own. A gtest ASSERT_* returns
from the middle of the test body, and a std::thread destroyed while still
joinable calls std::terminate: the run ends in SIGABRT, the assertion
message that names the real failure never reaches the report, and the
alarm-server child outlives the test holding its port.

An RAII guard owns the thread and runs cancel -> join, so the assertion
failure is what the report shows. Its stop() is idempotent, so the tests
still end the spin where they want the executor quiet.
The fixture's open62541 is built with UA_MULTITHREADING 0, so the server
may only be used from one thread. The command handler ran on a second
thread and wrote condition fields and triggered condition events there
while UA_Server_run iterated on the main thread; the mutex it held was
taken nowhere else, so it synchronised the handler against itself and
nothing against the server.

The reader thread now only queues command lines. The server is driven by
UA_Server_run_startup / run_iterate / run_shutdown, and each queued
command runs between iterations on that thread, so the address space and
the Alarms & Conditions subsystem have a single user. The reader ends on
EOF, which the harness produces by closing its write end of the pipe
before it signals the server.
The three helper FQNs are not all built the same way. The subscription
executor passes the gateway's own namespace, so its node follows the
gateway; the fault-client and lifecycle-reader nodes are built from the
gateway's node name alone and take the process default namespace. A remap
naming the gateway alone - "-r <gateway>:__ns:=/x" - moves the gateway and
the subscription node and leaves the other two behind, and the gateway
then served both of them as diagnosable apps and counted them as peers.

The predicate derives each helper's FQN from its own creation site and
matches both spellings for the two that do not follow the namespace.
…ared

The undeclared-node warning on GET /health is an instruction - "Declare
them in the manifest" - and it was naming the three nodes the gateway runs
in its own process. Declaring one produces a manifest app that the app
filter then removes, so the two halves contradicted each other.

The orphan scan skips the helper nodes, using the same predicate the app
filter uses, and a declared app bound to one of them is dropped with a
warning that names it instead of a debug line. A feature test drives a
hybrid gateway with "unmanifested_nodes: error" and pins both: the report
names an undeclared node of the test's own and none of the helpers.
The gateway creates its helper nodes at three points of start-up, two of
them after the first refresh and after /health starts answering. A settle
gate that waits for the gateway's own node to appear can therefore read a
list built from a graph that held no helper at all, and the absence
assertions then hold whatever the filter does.

The settle sequence now proves its own order: all three helpers on the
graph first, then a witness node created after them, then an /apps
snapshot that lists the witness - such a snapshot was built from a graph
that held the helpers too. The three stage budgets also sum well inside
the file's ctest timeout, so a failure reports its assertion instead of
being killed unnamed.

The config reference documents the rule: which nodes are filtered, why the
gateway's own node stays an App, how the namespaces work out, and what the
switch re-exposes when turned off.
…ase that failed

The quit branch closed this process's own stdin descriptor and claimed
that was what released the reader. It is not: close(2) does not unblock a
reader already parked in read(2), which a timed run confirms - with the
writer holding the pipe for eight seconds after quit, the process ended
after eight seconds, not at once. The call is gone and the comment names
the real terminator, the EOF the writer produces by closing its end.

The exit diagnostic named UA_Server_run, which the loop no longer calls.
Startup and shutdown each report their own bad status now. run_iterate has
no status to report - its return is how long the server may idle - so it
bounds the pause between iterations instead of being dropped.
Both cancel() and join() can throw and a destructor is implicitly
noexcept, so an escape from the guard's destructor would be the
std::terminate the guard exists to prevent. The destructor contains them
and prints what happened; by then the test has already recorded its
verdict, which is what the run has to report.

stop() also sets its idempotence flag after the join rather than before,
so a throwing cancel leaves the object willing to try again instead of
holding a thread nobody will join.
The fixture server succeeds at the requested sequence length, one element
per 100 ms tick, so the order sets how long the goal stays cancellable. At
20 it finishes on its own in under two seconds, which a loaded or
instrumented runner can spend on the create plus the cancel round trip,
and the assertions then describe a goal that completed rather than one
that was stopped. 50 is the largest order the server accepts.

The tracked-goal helper returns its optional instead of dereferencing it
after a non-fatal expectation, and the caller asserts, so a missing goal
is a readable failure rather than undefined behaviour.
The condition is a static misconfiguration - an app declared against one of
the gateway's helper nodes stays declared - while refresh_cache() runs on
every graph event and again on the backstop cadence, which the integration
fixtures set to one second. The warning repeated for the life of the
process. It is now gated on the set of offending apps changing, the same
one-shot discipline the entity-cache warning in the same function uses.

The predicate's doc comment and the config reference also write down what
the root-namespace spelling costs: a helper-named node at the root is
plumbing whichever gateway created it, because two gateways that keep the
default node name and differ only in namespace build the same fully
qualified name for it.
…ateway

The three helper node names take two shapes, and only one of them was
covered end to end. A remap naming the gateway alone moves it and the
subscription node into a namespace and leaves the fault-client and
lifecycle-reader nodes at the root, which is the shape the predicate
reconstructs rather than reads.

The new feature test launches exactly that invocation, asserts the split is
really on the graph, and pins the app list to the gateway plus the test's
own two nodes.
…es early

Two ways the guard could still reach std::terminate through its thread
member, which no catch in its destructor can intercept. A cancel() that
throws returned from stop() before the join, leaving the thread joinable.
And a cancel issued before spin() has begun is refused and lost, so the
thread spins on and the join never returns - a test that asserts early
enough hangs until ctest kills it.

The cancel is now contained and re-issued until the spin function itself
reports it returned, and only then is the thread joined. A test injects a
cancel that throws and reaches the end of the body, which under the old
shape ended the binary with SIGABRT instead.
The new predicate's declaration sat between filter_internal_node_apps' doc
block and the function it documents, so doxygen attached that block to the
predicate and reported four parameters it does not take.
rclcpp::Executor::cancel() is virtual on jazzy and later but not on humble,
where a derived executor's cancel() does not compile with `override` and
would not be the one called through a base reference anyway. The guard now
takes the cancel as a callable defaulting to the executor's own, and the
test passes one that throws after stopping the spin. Nothing derives from
the executor, so the same source builds on every distro.
@bburda
bburda force-pushed the fix/opcua-rescan-comms-lost-freeze-source branch from 0b47c79 to a791243 Compare September 13, 2026 18:34
@bburda
bburda marked this pull request as ready for review September 13, 2026 19:34
// the next reconnect re-derives, so the pending buffer may drop it before
// anything one-shot.
log_info(std::string("OPC-UA connection established, clearing any standing ") + kCommsLostFaultCode);
send_clear_fault(kCommsLostFaultCode, ClearOrigin::LinkState);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fault store keys PLC_COMMS_LOST by code alone and the S7/ADS/Modbus bridges raise the same code into the same fault manager, so this clear on first connect (and the poller's clear on every reconnect at opcua_poller.cpp:1216, no longer gated on comms_lost_raised_) flips another bridge's CONFIRMED outage to CLEARED every time the OPC UA link comes up, gateway start included. Before sending, GetFault the code and only clear when reporting_sources names this component, so the restart case is still healed without touching a fault this plugin never raised.

return pending.kind == PendingFaultDispatch::Kind::Clear && pending.fault_code == entry.fault_code;
});
if (same_code != buffer.end()) {
buffer.erase(same_code);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This erase runs before the capacity check and ignores a Report for the same code sitting after the older clear, so a buffered Report(A), Clear(A), Report(A), Clear(A) flushes as Report(A), Report(A), Clear(A): occurrence_count stays at 1, one OCCURRED audit record is lost, and with a confirmation threshold below -1 the two FAILEDs stack the debounce counter and confirm a fault the real stream never confirmed. Only coalesce when the newest pending entry for this code is already a Clear (search from the back, stop at the first entry with this code), so Clear, Clear from repeated reconnects still collapses.

const OpcuaClient::DeviceInfo & info,
const std::string & endpoint_url) {
const ComponentIdentity ci = derive_component_identity(info, endpoint_url);
if (ci.id.empty() || ci.id == current_id) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connect_generation bumps on every fresh connect (opcua_client.cpp:539), so this re-derives on a plain reconnect to the same PLC, not only after an adoption, and it accepts any different id: one empty or partial read_device_info on the first read of the new session renames siemens_ag_cpu_1505sp_f to opcua-192_168_1_10, moves the component URL and the _alarms entity and orphans the faults standing under the old id, and since component_identity_generation_ is stamped before the result is checked nothing retries until the next session. Only re-derive while the current id is still the endpoint-derived placeholder, never replace a nameplate-derived id with the host fallback.

// does not match the derived form and is left alone.
auto & auto_alarms = node_map_.mutable_auto_alarms();
if (auto_alarms.entity_id == previous_id + "_alarms") {
auto_alarms.entity_id.clear();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs on the poll thread and clears auto_alarms_.entity_id, then reassigns it and rebuilds entity_defs in finalize_auto_alarms_overlay, all under node_map_mutex_, but the poller never takes that mutex: its event pump thread delivers on_event from run_iterate and reads node_map_.auto_alarms().entity_id and find_by_node_id lock-free (opcua_poller.cpp:920, 975). The ConditionRefresh burst lands on exactly this first session, so a std::string read races a clear-then-reassign and the fault report can carry a garbage entity_id. Give the poller its own copy of the alarm config taken at subscribe time and swap it from the poll thread.

// device once a session is up, so an adopted PLC stops being served under the
// provisional endpoint-derived id. Runs BEFORE the re-walk below, which
// rebuilds entity_defs off the component id.
maybe_rederive_component_identity();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs too late for the alarms entity: in the reconnect arm the poller has already run setup_event_subscriptions() and ConditionRefresh before poll_callback_ fires, so conditions replayed on the adopted session get pinned in conditions_ (apply_condition_state keeps the first entity_id) and reported under <old-id>_alarms, an entity finalize_auto_alarms_overlay then drops from entity_defs, leaving those faults and all their later clears orphaned. Add a connected hook to PollerConfig (next to rediscover_endpoint) that the reconnect arm calls right after connect() and before setup_event_subscriptions(), and do the rename there.

// client owns the endpoint once connect() has been called with it, and its
// accessors are mutex-guarded.
const std::string current = client_ ? client_->endpoint_url() : client_config_.endpoint_url;
if (!chosen || *chosen == current) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only checks that the rescan hit is a different URL, so once the adopted PLC drops and any other None/Anonymous data server is reachable on the subnet (a second PLC, Kepware, an engineering station), the next rescan hands that server to the poller, which connects, logs "connection restored", clears PLC_COMMS_LOST and in config-less mode renames the component after the new device. The real outage vanishes from diagnostics, which the merge base never did because it retried the adopted endpoint forever. Only rescan while the current endpoint has never carried a session, or accept a hit only when its ApplicationUri matches the adopted server.

return pending.kind == PendingFaultDispatch::Kind::Clear && pending.link_state;
});
if (oldest_link_state != buffer.end()) {
buffer.erase(oldest_link_state);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This eviction assumes the next reconnect re-sends the clear, but when the clear was queued by a successful connect the link is up and no reconnect is coming: restart the gateway with a persisted PLC_COMMS_LOST while fault_manager is down, let 256 alarm edges arrive before it returns, and the flush leaves PLC_COMMS_LOST CONFIRMED against a healthy link until the next drop. The Refused branch below has the same hole. Set a flag when a link-state clear is evicted or refused and re-run clear_comms_lost_on_connect() from flush_pending_reports once the sink is ready.

// rclcpp::ok() only reads the default context's atomic shutdown flag, so it is
// safe to call from the set_context thread and the poll thread alike. The rule
// itself, and why both signals are needed, lives in discovery_cancelled_for.
return discovery_cancelled_for(shutdown_requested_.load(), rclcpp::ok());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rclcpp::ok() here reads the process-global default context, not the context the gateway node lives in, so any host that constructs GatewayNode with NodeOptions().context(...) gets discovery_cancelled() permanently true: the start-up sweep returns {} before a single probe and rescan_endpoint_for_reconnect bails at its top check, so the promised rescan never happens. Grab node->get_node_base_interface()->get_context() in set_context (as graph_watchdog_plugin.cpp:379 does), keep it as a member and pass it to rclcpp::ok(ctx).

const std::string self_fqn = node_ != nullptr ? node_->get_fully_qualified_name() : std::string();
for (const auto & rt_app : runtime_apps) {
if (rt_app.bound_fqn.has_value() && matched_nodes.find(rt_app.bound_fqn.value()) == matched_nodes.end()) {
if (is_own_gateway_helper_node(rt_app.bound_fqn.value(), self_fqn)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This skip runs regardless of discovery.runtime.filter_internal_nodes, but the app filter in refresh_cache is gated on that flag and discovery-options.rst says false re-exposes the three helpers. With the switch off the helpers are served as Apps yet never appear in /health's unmanifested_nodes or orphan_count, while _ros2cli_* nodes appear in both. Either gate this on the same setting or state in discovery-options.rst that the health exclusion is unconditional.

|| fail "renamed component reports endpoint '${endpoint}', expected the adopted server"
[[ "${connected}" == "True" ]] \
|| fail "renamed component reports connected='${connected}', expected a live session"
[[ "${renamed}" != "${FALLBACK_COMPONENT_ID}" ]] \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed can only be DEVICE_COMPONENT_ID here (set at 297, otherwise fail at 302 already exited), so this check cannot fail, and nothing asserts that opcua-localhost actually left /components, which is what the header and the comment at 305 say this pass is for. Assert FALLBACK_COMPONENT_ID is absent from ids after the rename. The RestartCount checks at 221 and 317 cannot fail either, the containers run with no --restart policy so it is always 0; compare .State.StartedAt before and after, or drop them.

…app filter is on

RuntimeLinker takes discovery.runtime.filter_internal_nodes. While it is
true the gateway's own helper nodes are left out of orphan_nodes, which is
what feeds linking.orphan_count and the unmanifested_nodes warning on
/health; with it false the helpers are served as Apps and counted like any
other undeclared node. discovery-options.rst and the health handler
comment state the coupling. Comments in the touched gateway and
integration-test files say what the code is.
…ep the PLC's order

The PLC_COMMS_LOST clear is a decision the plugin takes on the poll thread,
only while the link is up: it asks the fault manager who reported the
standing row and clears it only when every source is an id this bridge
reports under (the current component id and the stand-ins it assigned).
A row two bridges hold stays for an operator. One fault-store read is
outstanding at a time, stamped with the probe and the connection
generation; a read the store does not answer within
fault_service_timeout_ms is dropped and taken again; an answer from before
a link drop is discarded and the decision owed again; a link-state clear
the bounded buffer could not keep is owed and decided once the sink is
ready.

The pending buffer coalesces a clear onto an earlier pending clear for the
same code only when nothing was reported for that code in between, so
report, clear, report, clear flushes in that order.

Identity in config-less mode: the connect hook re-derives the component
name from the device before the session subscribes, so replayed
conditions land on the new entity; a nameplate replaces a stand-in and
nothing replaces a nameplate; an empty read renames a stand-in only when
the endpoint changed; reads are budgeted per session; conditions the
poller already pinned move to the new entity; the event path reads a copy
of the alarm routing owned by the poller.

The binding: the plugin remembers the ApplicationUri of the server a
session reached. A rescan while disconnected looks for that server at any
address and refuses every other; a different server at the bound address
is dropped at connect. The first adoption, a configured endpoint_url and
a server without an ApplicationUri are unconstrained, and the README says
so. A rescan that throws is caught on the poll thread. The cancel
predicate reads the node's own context.
…e restart checks

The config-less pass asserts that the stand-in component left /components
in the same sample that shows the renamed one. The containers run with no
restart policy, so a gateway that dies is reported by the endpoint and
component assertions; the RestartCount checks are gone. The deadline
follows RESCAN_INTERVAL_S like its siblings.
…, and own every reconnect

The plugin writes the ApplicationUri it is bound to into discovery.binding_file
(default /var/lib/ros2_medkit/opcua/binding, env OPCUA_DISCOVERY_BINDING_FILE,
empty disables) when the binding is established and reads it in configure(),
so a restart keeps the identity. The start-up sweep and every rescan then
select only the bound server, whatever its address, and a different server at
the bound address is dropped at connect with the outage left standing; the
refusal names the file as the recommissioning gesture. A configured
endpoint_url neither reads nor writes the file, and a server that publishes no
ApplicationUri is never persisted. The file is written through a temporary,
fsynced and renamed over the resolved target, holds one trimmed line, and a
BOM, a control character or a second line is refused with a warning.

The reconnect arm owns every reconnect: after each client iterate the session
state is read, a session the library re-opened underneath is dropped and handed
to the arm, which connects through its own path, runs the binding check and
re-creates the alarm subscription. Every path that marks the client
disconnected drops the dead session's subscription bookkeeping, so the
re-created monitored item delivers events again after a PLC reboot.

Unit tests cover the file's format and the write contract; end-to-end tests
against the fixture cover a restarted plugin refusing a foreign server at the
bound address, a configured endpoint_url ignoring the file, the start-up
sweep selecting the bound server over a lower address, a swap and an outage
under a config-less session, and a device alarm delivered after the bound
server reboots.

The identity E2E's ctest timeout is sized to one stuck test's wait budget on
top of twice the idle pace, with the arithmetic at the registration.
…check each built item

test_a_scoped_item_says_what_its_templated_sibling_says compares every entity
in both listings, counts built items per entity type, collection and scope,
requires a built item for every listed resource, and checks that the item at
each key names that resource. test_every_advertised_collection_is_served
follows every app's hrefs and requires a cache-built href per entity type.
The class waits for calibration's operation through REQUIRED_OPERATIONS and
polls temp_sensor's data with the discovery budget before comparing, so the
counters do not depend on how fast the runner propagates a service.
…identity check covers

The README binding section carries the file's path and format, the
discovery-only rule, the clearing gesture, the start-up sweep and rescan
behaviour, the reconnect ownership and the shapes it was proven in, the
probing cadence at a refused address, the last-eight refused URIs, one file
per plugin instance, the mount needed for a re-created container, and the
configuration and environment rows.
… the plugin's own threads

The ReportFault, ClearFault and GetFault clients live in a callback group
that is not added to an executor together with the node. A single-threaded
executor owned by the plugin serves that group from a thread the plugin
starts in set_context. shutdown stops the poller, stops and joins that
thread, and then drops all three clients on the calling thread, so a client
destructor never runs on a gateway executor thread while another plugin or
the gateway creates an entity on the same node. The GetFault answer is
parked from the plugin's thread under the probe state's mutex, as before.

The client thread also ends once the node's context has shut down.
set_context is documented as called once per plugin instance.

A new end-to-end case builds two plugin instances in sequence on one node
while the gateway's executor spins, and checks that the first shutdown
joins the client thread and that the second instance's clients reach the
fault services.
EntityDetailStatusLinkTest and LifecycleHandlersWithProviderTest initialise
rclcpp when it is not running, and their TearDownTestSuite now shuts it down
the way LifecycleHandlersTest does. The test process exits with no rclcpp
context left initialised, so static destruction does not run alongside live
middleware threads.
…and name the test stubs' parameters

The GetFault answer and the fault_service_timeout_ms config value are read
through const references, and the persisted-binding and rescan results are
returned from non-const locals so they are moved out. The severity band
order check is written as the direct comparison of the integer bands.

The GetFault response callback keeps its future by value, because rclcpp
offers the response-callback overload of async_send_request only to a
callable taking exactly SharedFuture; the check is suppressed at that line
with the reason next to it.

In the identity tests, the stub definitions name every parameter, the child
process fixture and the rclcpp scope guard are neither copyable nor movable,
and the fault store stub's service callbacks take their arguments by const
reference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

opcua: rescan while disconnected, heal PLC_COMMS_LOST on connect, and name a plugin freeze-frame's source

2 participants